Move to grpc-dotnet #517

xlegalles · 2023-05-17T16:19:14Z

It targets net5.0;net6.0;net7.0 but I don't know if net5.0 is still relevant (not supported any more).
Also, tests only target net7.0.
I propose to update this package to v2.0 but of course, you decide.

I have tried to remove as many warnings as possible.

All unit tests now use a standard host instead of a Grpc.Core.Server which does not exist anymore. In that context, GatewayTestService cannot be resolved from DI. This is why I have created IRequestHandlerRegistration which encapsulate the logic for tests: it's a singleton component whose state can be reset.
In BaseZeebeTest, I have changed the lifecycle of this host and made it unique for all tests: the execution of tests is of course much faster.

The tricky part was about certificates and I've put some comments in ZeebeSecureClientBuilder.

One important remark in ZeebeClient: because the certificates we use for tests are untrusted, I have to add a RemoteCertificateValidationCallback line 86 to bypass validation. I'm not sure if this is relevant? In our case, we often use OpenSSL certificates and we have to use such a validation. But at a consequence the ShouldFailOnWrongCert UT does not work anymore.

I'm available for any discussion/comments but maybe not before 21th as we have public holidays in France.

CLAassistant · 2023-05-17T16:19:21Z

All committers have signed the CLA.

# Conflicts: # Client/Client.csproj

…tNetMigration

…tNetMigration # Conflicts: # Client.IntegrationTests/ZeebeIntegrationTestHelper.cs

xlegalles · 2023-05-22T12:45:46Z

resolves #348

# Conflicts: # Client.Cloud.Example/Client.Cloud.Example.csproj # Client.Examples/Client.Examples.csproj # Client.IntegrationTests/Client.IntegrationTests.csproj # Client/Client.csproj

xlegalles · 2023-05-31T07:55:35Z

Ok, I stop working on this PR until you have a bit of time for review. I cannot fix conflicts everyday, it's not possible. Everybody knows that a PR/branch must not live too long otherwise it becomes unmanageable and this is exactly what happens... A bit upset, I admit.

ChrisKujawa

Hey @xlegalles first thanks that you did this and provided a solution for it. Right now I'm have not really time to maintain this project, we try to find someone else to take over that. Furthermore, I was off the last few weeks (dependabot gets merged automatically), so sorry for the late response.

I haven't looked at all details in your PR, but what I have seen is that there are a lot of changes that change code style and move classes/files. This makes it quite hard to review (it is not clear what are the important parts and what not).. Over 1k lines of changes are not optimal to review in general. Would be nice if we could avoid the style changes. If we need them let's either create a separate commit for it or better create a separate PR for it.

Furthermore, please follow the contribution guide, and adjust your commit messages etc.

xlegalles · 2023-05-31T16:42:45Z

Ok, I'm going to revert some changes that are mostly related to code style. A lot because now namespaces can be defined without any brace, which is a nice improvement but changes all file content....

# Conflicts: # Client.Examples/Client.Examples.csproj # Client.IntegrationTests/Client.IntegrationTests.csproj # Client.UnitTests/Client.UnitTests.csproj

deps(client): uses Grpc.Net instead of Grpc.Core fix: camunda-community-hub#348

…tNetMigration

xlegalles · 2023-05-31T17:39:58Z

Client.UnitTests/ZeebeClientTest.cs

This test class has been splitted because some tests require a test server while others do not need any.

…tNetMigration

ChrisKujawa

Hey @xlegalles

sorry that it took me so long. I'm quite busy with other stuff (zeebe etc.)

I have some questions which we should clarify before we continue I think, see below.

I used the emoji code for the review, which we use also @ Zeebe. For more information take a look here https://github.com/camunda/zeebe/blob/main/CONTRIBUTING.md#reviewing-a-pull-request

ChrisKujawa · 2023-07-03T19:20:10Z

Client.UnitTests/CancelProcessInstanceTest.cs

@@ -18,6 +19,7 @@ public async Task ShouldSendRequestAsExpected()
            {
                ProcessInstanceKey = 12113
            };
+            TestService.Reset();


❓ For what these resets have been added?

I have changed the logic of these tests and share the same server across all. The execution is now way more faster. But I need to reset its state, otherwise the expectation is polluted by previous methods.

ChrisKujawa · 2023-07-03T19:21:12Z

Client/Client.csproj

    <PackageReference Include="Grpc.Tools" version="2.55.1">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
    </PackageReference>
-    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="7.0.0" />


❓ There have been some dependencies removed. How does this work now?

The restoration process is able to manage transitive dependencies so that you don't need to specify them. If you do you will have to manage them, and ensure they are consistent with your primary dependencies.

ChrisKujawa · 2023-07-03T19:23:09Z

Client/Impl/Builder/ZeebeClientBuilder.cs

+            return new ZeebeClient(Address, Credentials, KeepAlive, SleepDurationProvider, LoggerFactory, Certificate);
+        }
+
+        private X509Certificate2 CreateFromPemFile(string fileName)


❌ Feels like this shouldn't be part of this PR, is this another feature you want to get merged?

Ok nevermind I have re-read our PR description:

I guess it is this:

The tricky part was about certificates and I've put some comments in ZeebeSecureClientBuilder.

One important remark in ZeebeClient: because the certificates we use for tests are untrusted, I have to add a RemoteCertificateValidationCallback line 86 to bypass validation. I'm not sure if this is relevant? In our case, we often use OpenSSL certificates and we have to use such a validation. But at a consequence the ShouldFailOnWrongCert UT does not work anymore.

Yes this is the explanation.

ChrisKujawa · 2023-07-03T19:26:13Z

Client/Impl/CustomHeaderInterceptor.cs

+    ///     The "user-agent" is already filled by grpc-dotnet
+    ///     typically something like: key=user-agent, value=grpc-dotnet/2.53.0 (.NET 7.0.5; CLR 7.0.5; net7.0; windows; x64)
+    /// </summary>
+    internal class CustomHeaderInterceptor : Interceptor


❓ Was this extracted? Why the change of the user agent?

I wanted to maintain the behavior that you have implemented with a specific user-agent description. If I don't do this you will receive on the server, the generic description of a .net grpc agent.

ChrisKujawa · 2023-07-15T07:21:14Z

Hey @xlegalles thank you for your time and effort!

I had a deeper look at this issue and it seemed to me that most of the changes were not really necessary.

I made separate PRs to migrate to .net 7 #543 and to migrate to grpc-dotnet #545, both involved not much (or none) refactoring.

I will implement/add some more features which have been recently added to Zeebe and then release a v2.0. version.

Thanks again and sorry that I was not able to merge your PR. But I hope that you're happy with the end result anyway and it helps you in your project as well.

P.S.:

Small recommendation, for the next time, please try to create smaller PRs (with fewer changes) it makes it easier to review and reason about. For example, try to split bigger changes up. Plus try to discuss the changes you want to do first in a separate issue, so you not working on changes unnecessarily which might not get accepted or merged.

Thank you again 🙇🏼

xlegalles · 2023-07-15T07:59:47Z

Hi Christopher, I understand your point of view and will do differently next time. And of course, I'm satisfied that we can use the new version. Thank you Xavier Le sam. 15 juil. 2023, 09:21, Christopher Kujawa (Zell) < ***@***.***> a écrit :

…

Hey @xlegalles <https://github.com/xlegalles> thank you for your time and effort! I had a deeper look at this issue and it seemed to me that most of the changes were not really necessary. I made separate PRs to migrate to .net 7 #543 <#543> and to migrate to grpc-dotnet #545 <#545>, both involved not much (or none) refactoring. I will implement/add some more features which have been recently added to Zeebe and then release a v2.0. version. Thanks again and sorry that I was not able to merge your PR. But I hope that you're happy with the end result anyway and it helps you in your project as well. P.S.: Small recommendation, for the next time, please try to create smaller PRs (with fewer changes) it makes it easier to review and reason about. For example, try to split bigger changes up. Plus try to discuss the changes you want to do first in a separate issue, so you not working on changes unnecessarily which might not get accepted or merged. — Reply to this email directly, view it on GitHub <#517 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABYFC7JFWE6WLAVAMOINUDXQJAHNANCNFSM6AAAAAAYFKPCU4> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

Move to grpc-dotnet

416c641

xlegalles requested a review from ChrisKujawa as a code owner May 17, 2023 16:19

xlegalles added 7 commits May 22, 2023 10:48

Merge branch 'main' into grpcDotNetMigration

2ebf8ce

# Conflicts: # Client/Client.csproj

Updated pipeline to .NET 7

8ab129a

Updated pipeline to .NET 7

aa768f9

Merge remote-tracking branch 'origin/grpcDotNetMigration' into grpcDo…

5bb68d6

…tNetMigration

fix integration tests

dfa51bd

fix integration tests

28c9f62

Merge remote-tracking branch 'origin/grpcDotNetMigration' into grpcDo…

93529d6

…tNetMigration # Conflicts: # Client.IntegrationTests/ZeebeIntegrationTestHelper.cs

xlegalles mentioned this pull request May 22, 2023

Migrate to grpc-dotnet #348

Closed

xlegalles added 3 commits May 26, 2023 10:18

Merge branch 'main' into grpcDotNetMigration

2a4909e

Merge branch 'main' into grpcDotNetMigration

bf3d81b

# Conflicts: # Client.Cloud.Example/Client.Cloud.Example.csproj # Client.Examples/Client.Examples.csproj # Client.IntegrationTests/Client.IntegrationTests.csproj # Client/Client.csproj

Merge error

9f85af7

ChrisKujawa reviewed May 31, 2023

View reviewed changes

xlegalles added 4 commits May 31, 2023 19:10

Merge branch 'main' into grpcDotNetMigration

8d793ee

# Conflicts: # Client.Examples/Client.Examples.csproj # Client.IntegrationTests/Client.IntegrationTests.csproj # Client.UnitTests/Client.UnitTests.csproj

build: uses .NET SDK 7.0

ff36c1e

deps(client): uses Grpc.Net instead of Grpc.Core fix: camunda-community-hub#348

build: uses .NET SDK 7.0

f6c67e0

deps(client): uses Grpc.Net instead of Grpc.Core fix: camunda-community-hub#348

Merge remote-tracking branch 'origin/grpcDotNetMigration' into grpcDo…

75fed8e

…tNetMigration

xlegalles commented May 31, 2023

View reviewed changes

xlegalles added 3 commits June 1, 2023 08:18

Merge remote-tracking branch 'origin/grpcDotNetMigration' into grpcDo…

e320a6f

…tNetMigration

Merge remote-tracking branch 'origin/grpcDotNetMigration' into grpcDo…

3468a00

…tNetMigration

Merge branch 'main' into grpcDotNetMigration

e88a9a6

ChrisKujawa reviewed Jul 3, 2023

View reviewed changes

ChrisKujawa mentioned this pull request Jul 13, 2023

Migrate to .net 7.0 #543

Merged

ChrisKujawa closed this Jul 15, 2023

ChrisKujawa mentioned this pull request Aug 10, 2023

Migrate to GrpcChannel #556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to grpc-dotnet #517

Move to grpc-dotnet #517

xlegalles commented May 17, 2023

CLAassistant commented May 17, 2023 •

edited

Loading

xlegalles commented May 22, 2023

xlegalles commented May 31, 2023

ChrisKujawa left a comment

xlegalles commented May 31, 2023 •

edited

Loading

xlegalles May 31, 2023

ChrisKujawa left a comment

ChrisKujawa Jul 3, 2023

xlegalles Jul 4, 2023

ChrisKujawa Jul 3, 2023

xlegalles Jul 4, 2023

ChrisKujawa Jul 3, 2023

ChrisKujawa Jul 3, 2023 •

edited

Loading

xlegalles Jul 4, 2023

ChrisKujawa Jul 3, 2023

xlegalles Jul 4, 2023

ChrisKujawa commented Jul 15, 2023 •

edited

Loading

xlegalles commented Jul 15, 2023 via email

Move to grpc-dotnet #517

Move to grpc-dotnet #517

Conversation

xlegalles commented May 17, 2023

CLAassistant commented May 17, 2023 • edited Loading

xlegalles commented May 22, 2023

xlegalles commented May 31, 2023

ChrisKujawa left a comment

Choose a reason for hiding this comment

xlegalles commented May 31, 2023 • edited Loading

Choose a reason for hiding this comment

ChrisKujawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisKujawa Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisKujawa commented Jul 15, 2023 • edited Loading

xlegalles commented Jul 15, 2023 via email

CLAassistant commented May 17, 2023 •

edited

Loading

xlegalles commented May 31, 2023 •

edited

Loading

ChrisKujawa Jul 3, 2023 •

edited

Loading

ChrisKujawa commented Jul 15, 2023 •

edited

Loading